Skip to content

Conversation

@beena352
Copy link

Summary of the Pull Request

Adds localization support to wsladiag by replacing all hardcoded user-facing strings with localized messages from Resources.resw. All messages (usage, errors, session list output) are now accessible via the Localization class.

PR Checklist

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • [x ] Tests: Added/updated if needed and all pass
  • [x ] Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

  • Added 11 new localization entries to [Resources.resw] for wsladiag-specific messages
  • Updated [wsladiag.cpp] to use [Localization::MessageWslaXxx()] for all user-facing output including:
    Usage information
  • Session not found / open session failed errors
  • Session list output (headers, no sessions message, session count)
  • Shell exit status messages
  • Unknown command errors
  • Added [using wsl::shared::Localization;]declaration following existing codebase patterns
  • All existing WsladiagTests pass (8/8 tests validate localized message output)

Validation Steps Performed

Built project successfully with cmake --build build-vs-x64 --config Debug --parallel
Ran all wsladiag tests: [bin\x64\debug\test.bat /name:WsladiagTests]- All 8 tests passed
Manually tested commands:
[wsladiag.exe --help] - displays localized usage
wsladiag.exe list - displays localized session list or "No WSLA sessions found"
wsladiag.exe shell InvalidSession - displays localized error "Session not found: 'InvalidSession'"
wsladiag.exe badcommand - displays localized "Unknown command: 'badcommand'" error

@beena352 beena352 force-pushed the user/beenachauhan/wsladiag-localization-support branch from ddebff2 to 1107c6a Compare December 30, 2025 21:28
Copy link
Member

@benhillis benhillis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will want to add WSLA strings to a different .resw file. Otherwise we will have localized strings in both products that do not need to be present.

@OneBlue
Copy link
Collaborator

OneBlue commented Dec 31, 2025

I think we will want to add WSLA strings to a different .resw file. Otherwise we will have localized strings in both products that do not need to be present.

We might be able to get away with it either by relying on the fact that the localization methods are inline, and so the compiler should eliminate the strings that aren't used, or by having a special prefix for WSLA strings so the localization script can generate two separate headers.

Either way, that's something that would be ok to do in a followup PR IMO

@beena352 beena352 marked this pull request as ready for review January 1, 2026 20:18
@beena352 beena352 requested a review from a team as a code owner January 1, 2026 20:18
Copilot AI review requested due to automatic review settings January 1, 2026 20:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request adds comprehensive localization support to the wsladiag diagnostics tool by replacing all hardcoded user-facing strings with localized messages from Resources.resw. The changes ensure that error messages, usage information, and session listing output can be properly localized for international users.

Key Changes

  • Replaced hardcoded strings with 11 new localization entries for wsladiag-specific messages
  • Refactored usage information to use PrintUsage() function with localized message
  • Updated error handling to use consistent localization API patterns
  • Improved table formatting to properly handle localized column headers with dynamic width calculation

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/windows/wsladiag/wsladiag.cpp Replaced all hardcoded user-facing strings with Localization API calls; added using declarations; renamed variable from 'code' to 'exitCode' for clarity; changed ErrorCodeToString to GetErrorString; improved table formatting logic
localization/strings/en-US/Resources.resw Added 11 new message entries (MessageWsladiagUsage, MessageWslaSessionNotFound, MessageWslaOpenSessionFailed, MessageWslaNoSessionsFound, MessageWslaSessionsFound, MessageWslaShellExited, MessageWslaShellSignalled, MessageWslaHeaderId, MessageWslaHeaderCreatorPid, MessageWslaHeaderDisplayName, MessageWslaUnknownCommand) with appropriate localization annotations

wslutil::PrintMessage(std::format(L"Found {} WSLA session{}:", sessions.size(), sessions.size() > 1 ? L"s" : L""), stdout);
wslutil::PrintMessage(Localization::MessageWslaSessionsFound(sessions.size()), stdout);

// Compute column widths from headers + data .
Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an extra space before the period in this comment. It should be "headers + data." instead of "headers + data ."

Suggested change
// Compute column widths from headers + data .
// Compute column widths from headers + data.

Copilot uses AI. Check for mistakes.
@beena352 beena352 force-pushed the user/beenachauhan/wsladiag-localization-support branch from 46dfad2 to 57c5c1d Compare January 2, 2026 19:56
@benhillis
Copy link
Member

I think we will want to add WSLA strings to a different .resw file. Otherwise we will have localized strings in both products that do not need to be present.

We might be able to get away with it either by relying on the fact that the localization methods are inline, and so the compiler should eliminate the strings that aren't used, or by having a special prefix for WSLA strings so the localization script can generate two separate headers.

Either way, that's something that would be ok to do in a followup PR IMO

Ah all of the languages strings are embedded in the binaries themselves? Then yeah, this is probably ok.

@beena352 beena352 force-pushed the user/beenachauhan/wsladiag-localization-support branch from 57c5c1d to 89db2ad Compare January 5, 2026 23:35
Copy link
Collaborator

@OneBlue OneBlue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ! Thank you for doing this.

@benhillis benhillis self-requested a review January 6, 2026 00:27
Copy link
Member

@benhillis benhillis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think copilot identified something real

@beena352 beena352 force-pushed the user/beenachauhan/wsladiag-localization-support branch from 89db2ad to 846648e Compare January 6, 2026 23:01
@beena352
Copy link
Author

beena352 commented Jan 6, 2026

I think copilot identified something real

Thanks for the heads-up! If you can point me to what Copilot flagged, I’ll take a look.

@benhillis benhillis requested a review from Copilot January 7, 2026 17:51
@benhillis
Copy link
Member

@beena352 there are comments in this PR with copilots findings.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.


using namespace wsl::shared;
namespace wslutil = wsl::windows::common::wslutil;
using wsl::shared::Localization;
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The using wsl::shared::Localization; declaration is redundant since using namespace wsl::shared; on line 24 already brings the Localization class into scope. This can be removed for clarity.

Suggested change
using wsl::shared::Localization;

Copilot uses AI. Check for mistakes.
Comment on lines 27 to 28
using wsl::windows::common::Context;
using wsl::windows::common::ExecutionContext;
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The using declarations for Context and ExecutionContext are added but these types do not appear to be used anywhere in this file. These should be removed unless they are actually needed.

Suggested change
using wsl::windows::common::Context;
using wsl::windows::common::ExecutionContext;

Copilot uses AI. Check for mistakes.
<value>No WSLA sessions found.</value>
</data>
<data name="MessageWslaSessionsFound" xml:space="preserve">
<value>Found {} WSLA session(s):</value>
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The localized message always includes "(s)" regardless of the count, which changes the output behavior from the original code. The original code conditionally added "s" only when sessions.size() > 1, producing "Found 1 WSLA session:" or "Found 2 WSLA sessions:". The new version will always show "Found 1 WSLA session(s):" which is less grammatically natural. Consider using the conditional plural pattern to maintain the original behavior, or verify that this change is intentional.

Suggested change
<value>Found {} WSLA session(s):</value>
<value>Found {} WSLA session{}:</value>

Copilot uses AI. Check for mistakes.
@beena352 beena352 force-pushed the user/beenachauhan/wsladiag-localization-support branch from 846648e to f08bf3a Compare January 7, 2026 17:59
@beena352 beena352 force-pushed the user/beenachauhan/wsladiag-localization-support branch from f08bf3a to ec16460 Compare January 7, 2026 22:42
@beena352
Copy link
Author

beena352 commented Jan 7, 2026

@beena352 there are comments in this PR with copilots findings.

Thanks! I picked up the Copilot suggestions that fit the current scope.

@benhillis benhillis self-requested a review January 7, 2026 23:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment on lines 1882 to 1916
<data name="MessageWslaSessionNotFound" xml:space="preserve">
<value>Session not found: '{}'</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="MessageWslaOpenSessionFailed" xml:space="preserve">
<value>OpenSessionByName('{}') failed</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="MessageWslaNoSessionsFound" xml:space="preserve">
<value>No WSLA sessions found.</value>
</data>
<data name="MessageWslaSessionsFound" xml:space="preserve">
<value>Found {} WSLA session{}:</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="MessageWslaShellExited" xml:space="preserve">
<value>{} exited with: {}</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="MessageWslaShellSignalled" xml:space="preserve">
<value> (signalled)</value>
</data>
<data name="MessageWslaHeaderId" xml:space="preserve">
<value>ID</value>
</data>
<data name="MessageWslaHeaderCreatorPid" xml:space="preserve">
<value>Creator PID</value>
</data>
<data name="MessageWslaHeaderDisplayName" xml:space="preserve">
<value>Display Name</value>
</data>
<data name="MessageWslaUnknownCommand" xml:space="preserve">
<value>Unknown command: '{}'</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent indentation: The data elements starting from line 1882 onwards are missing the standard 2-space indentation used throughout the rest of the file. All data tags should be indented with 2 spaces to match the existing formatting convention.

Suggested change
<data name="MessageWslaSessionNotFound" xml:space="preserve">
<value>Session not found: '{}'</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="MessageWslaOpenSessionFailed" xml:space="preserve">
<value>OpenSessionByName('{}') failed</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="MessageWslaNoSessionsFound" xml:space="preserve">
<value>No WSLA sessions found.</value>
</data>
<data name="MessageWslaSessionsFound" xml:space="preserve">
<value>Found {} WSLA session{}:</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="MessageWslaShellExited" xml:space="preserve">
<value>{} exited with: {}</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="MessageWslaShellSignalled" xml:space="preserve">
<value> (signalled)</value>
</data>
<data name="MessageWslaHeaderId" xml:space="preserve">
<value>ID</value>
</data>
<data name="MessageWslaHeaderCreatorPid" xml:space="preserve">
<value>Creator PID</value>
</data>
<data name="MessageWslaHeaderDisplayName" xml:space="preserve">
<value>Display Name</value>
</data>
<data name="MessageWslaUnknownCommand" xml:space="preserve">
<value>Unknown command: '{}'</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="MessageWslaSessionNotFound" xml:space="preserve">
<value>Session not found: '{}'</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="MessageWslaOpenSessionFailed" xml:space="preserve">
<value>OpenSessionByName('{}') failed</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="MessageWslaNoSessionsFound" xml:space="preserve">
<value>No WSLA sessions found.</value>
</data>
<data name="MessageWslaSessionsFound" xml:space="preserve">
<value>Found {} WSLA session{}:</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="MessageWslaShellExited" xml:space="preserve">
<value>{} exited with: {}</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="MessageWslaShellSignalled" xml:space="preserve">
<value> (signalled)</value>
</data>
<data name="MessageWslaHeaderId" xml:space="preserve">
<value>ID</value>
</data>
<data name="MessageWslaHeaderCreatorPid" xml:space="preserve">
<value>Creator PID</value>
</data>
<data name="MessageWslaHeaderDisplayName" xml:space="preserve">
<value>Display Name</value>
</data>
<data name="MessageWslaUnknownCommand" xml:space="preserve">
<value>Unknown command: '{}'</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>

Copilot uses AI. Check for mistakes.
@beena352 beena352 merged commit 0665546 into microsoft:feature/wsl-for-apps Jan 8, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants